Optional Top Level Keys and Configuration Log Option #96
Conversation
| return bool() | ||
| elif key == []: | ||
| return list() | ||
| elif key == OrderedDict(): |
There was a problem hiding this comment.
This smells funny. As you point out, lists and dicts cannot be keys in a Python dictionary. So why do you allow them as your own keys? Why isn't it just key == 'list' and key == 'dict'?
Keep in mind, this will only match if the key is an empty OrderedDict. Do you mean to use type?
Finally, if key == OrderedDict(), you return dict(), meaning you've removed the ordering. Why the discrepancy in types?
There was a problem hiding this comment.
@austinbyers it's not key == 'list' or 'dict' because in the schema's they are dictated as [] or {}. Since we load as an OrderedDict, empty {} is converted to OrderedDict(). It's mean't to be empty.
| json_payload[key_name] = default_optional_values(value_type) | ||
|
|
||
| # Handle jsonpath extraction of records | ||
| if config_options and len(config_options) and records_schema: |
There was a problem hiding this comment.
The len check is redundant: if config_options returns True only if config_options is not None and is non-empty
test/unit/test_classifier.py
Outdated
| 'kinesis': { | ||
| 'data': base64.b64encode(kinesis_data) | ||
| } | ||
| } |
There was a problem hiding this comment.
This indentation doesn't any sense to me
|
Thanks for the review @austinbyers, I just addressed your comments |
|
LGTM |
Instead of having optional top level keys for a log type, move all options into 'configuration'. This includes KV/CSV delimiter/separator, json_path, and envelope keys.
The goal of optional_top_level_keys is to provide a mechanism for a more flexible schema. If the key(s) described in optional_top_level_keys exist in the incoming record, they are simply type checked. If they do not exist, a default value is created in place to avoid rules calling keys that do not exist.
…evel_keys support
jacknagz
force-pushed
the
jacknagz-top-level-keys-and-parser-config-opts
branch
from
f1c67b9 to
bd062ea
Compare
to: @airbnb/streamalert-maintainers
size: medium
resolves #95
Library Changes
conf/logtype options under a new key calledconfigurationhintsstays at the top level, since it's not an option, but rather a guard.delimiter,separatormove underconfiguration.envelopeis now calledenvelope_keysand nested underconfiguration.recordsis now calledjson_pathand also nested underconfiguration.configurationkey calledoptional_top_level_keys.pylintoffenses.Other Changes
autopephelper scriptunit_test.shscript to test thestream_alert_clipackageNotes
conf/logs.jsonsettings if the keys are not updated as described above.